android: detect native ABI and API level correctly#24868
Conversation
lunagl
force-pushed
the
android-api-detection
branch
from
cd92d62 to
bf8be8d
Compare
|
Would be good to know what kind of Android system(s) this has been tested on. |
lunagl
force-pushed
the
android-api-detection
branch
from
bf8be8d to
a0c77fe
Compare
|
I have tested this on a Google Pixel 7 running Android 16 (API 36). Since it only relies on |
lunagl
force-pushed
the
android-api-detection
branch
from
a0c77fe to
b31d075
Compare
lunagl
force-pushed
the
android-api-detection
branch
from
b31d075 to
823c738
Compare
| } | ||
|
|
||
| fn detectAndroidApiLevel() !u32 { | ||
| var alloc_buf: [384]u8 = undefined; |
There was a problem hiding this comment.
Why this buffer size in particular?
There was a problem hiding this comment.
Technically from what I can tell only argv plus it's elements and envp (which is empty) need to be allocated, but because spawn uses an ArenaAllocator it was difficult to determine the exact memory required (and I don't know if it's 100% deterministic, I believe 256 worked once and then failed on another try), so I started with 128, tried 256, and then 384 (256 + 128) as a value that didn't OOM. It's not super pretty I agree, but since we don't have a proper heap allocator here I'm not sure what else to do
There was a problem hiding this comment.
I don't know a good strategy for this either, but if this is a heuristic/guessed value, then a comment that documents this fact for the future would be helpful.
There was a problem hiding this comment.
Maybe we should just bite the bullet and make resolveTargetQuery take an allocator? I haven't looked in depth, but a quick grep suggests most call sites have an allocator of some sort available.
There was a problem hiding this comment.
I don't think my understanding of the codebase is deep enough to make that call, I could certainly implement it tho. Right now it would only be used for the API level detection, I'm not sure if other parts of resolveTargetQuery would benefit from an allocator as well. What do you think?
There was a problem hiding this comment.
If you want to take a stab at it, please go ahead, though I suggest doing it as a separate commit within the PR.
lunagl
force-pushed
the
android-api-detection
branch
5 times, most recently
from
ebc9a98 to
a59f1ff
Compare
lunagl
force-pushed
the
android-api-detection
branch
3 times, most recently
from
4de90da to
467ac61
Compare
lunagl
force-pushed
the
android-api-detection
branch
from
467ac61 to
63ef488
Compare
lunagl
force-pushed
the
android-api-detection
branch
2 times, most recently
from
c21cf94 to
7e00967
Compare
ABI detection previously did not take into account the non-standard directory structure of Android. This has been fixed. The API level is detected by running `getprop ro.build.version.sdk`, since we don't want to depend on bionic, and reading system properties ourselves is not trivially possible.
For now the allocator is only used in `detectAndroidApiLevel`, but there are probably other places in the detection logic that could benefit from this too.
lunagl
force-pushed
the
android-api-detection
branch
from
7e00967 to
7a72618
Compare
|
I'm trying to figure out why the pipelines are failing here, the errors seem unrelated to my changes. Any ideas? |
|
It's happening because we previously didn't exercise libc-less process spawning logic as part of the standard library tests, it seems. The issue is #23262. |
|
This pull request is not ready for review because:
Since we have moved development to Codeberg, please open your pull request there if you would like to continue these efforts. |
5 participants
Fixes ABI detection and adds API level detection (#24630).
I was planning on also including a fix for #24860, but I am not sure how to proceed due to the
faccessatsyscall not supporting flags (#16606).